Skip to content
This repository was archived by the owner on Jul 3, 2020. It is now read-only.

Update everything to ES6 and adopt a code style#120

Merged
facekapow merged 23 commits intomasterfrom
code-style
Jul 29, 2016
Merged

Update everything to ES6 and adopt a code style#120
facekapow merged 23 commits intomasterfrom
code-style

Conversation

@facekapow
Copy link
Copy Markdown
Contributor

@facekapow facekapow commented Jul 8, 2016

This PR is a big one, it changes pretty much all of the JavaScript files, since it updates everything to ES6, plus adopts airbnb/javascript as a code style (with a few tweaks, like using require instead of ES6 modules).

Done so far:

  • js/*.js
  • js/component
    • js/component/dns-client
  • js/core
    • js/core/*.js
    • js/core/debug
    • js/core/keyboard
    • js/core/net
    • js/core/pci
    • js/core/ps2
    • js/core/random
    • js/core/stdio
    • js/core/tty
  • js/driver
    • js/driver/ps2
    • js/driver/virtio
      • js/driver/virtio/vring
      • js/driver/virtio/*.js
  • js/modules
  • js/service
    • js/service/dhcp-client
    • js/service/dns-resolver
    • js/service/shell
  • js/test
    • js/test/unit
      • js/test/unit/buffers
      • js/test/unit/lib
      • js/test/unit/net
      • js/test/unit/platform
      • js/test/unit/random
      • js/test/unit/script
      • js/test/unit/timers
      • js/test/unit/virtio
  • js/utils

@vitkarpov
Copy link
Copy Markdown

vitkarpov commented Jul 8, 2016

Goodbye commit history :)

@facekapow
Copy link
Copy Markdown
Contributor Author

Also added guidelines for contributing, plus a list of exceptions to Airbnb's style guide.

@RossComputerGuy
Copy link
Copy Markdown

How do we update runtime with npm.

@facekapow facekapow changed the title Update everything to ES6 and adopt a code style - WIP Update everything to ES6 and adopt a code style Jul 17, 2016
@facekapow
Copy link
Copy Markdown
Contributor Author

This is done and ready for review/merge, the only thing I'm not really confident about is the CONTRIBUTING.md. I've got some more node compatibility fixes, but I'd prefer to add them in another PR after this has been merged to avoid any conflicts.

@iefserge
Copy link
Copy Markdown
Member

This is awesome, I'll do some tests too, to make sure everything works.

@facekapow facekapow mentioned this pull request Jul 22, 2016
Comment thread js/component/dns-client/dns-packet.js Outdated

function readHostname(reader) {
var labels = [];
const readHostname = (reader) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think regular function declarations are fine, stack trace will be nicer

Comment thread js/driver/virtio/net.js Outdated
if (!dev.writeGuestFeatures(features, driverFeatures, deviceFeatures)) {
debug('[virtio] driver is unable to start');
return;
return debug('[virtio] driver is unable to start');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, it shouldn't return debug() return value

@facekapow
Copy link
Copy Markdown
Contributor Author

facekapow commented Jul 24, 2016

Ok, I've update the code to only use arrow functions for property functions and callbacks and function declarations for everything else, I've run the code through a beautifier, removed the newlines in js/__loader.js, used spaces instead of newlines in js/core/pci/scan.js, readded eslint-plugin-runtime-internal, disabled no-use-before-define for functions, and fixed the other issues you mentioned.
And I tested it and it still works.

Comment thread js/driver/virtio/net.js Outdated
const { MACAddress, Interface } = runtime.net;

const virtioHeader = (() => {
const virtioHeader = (function () { // eslint-disable-line wrap-iife
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick, arrow function if ok in this case?

@facekapow
Copy link
Copy Markdown
Contributor Author

facekapow commented Jul 25, 2016

Converted IIFE functions to arrow functions, and fixed the minor style issues. Sorry, most of these little things are because of the beautifier I ran it through.

Comment thread js/test/unit/net/tcp.js Outdated
socket._transmit = recvACKFIN;
socket.close();
t.equal(socket._state, tcpSocketState.STATE_TIME_WAIT);
socket._transmit = recvACKFIN; socket.close(); t.equal(socket._state, tcpSocketState.STATE_TIME_WAIT);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single line?

@iefserge
Copy link
Copy Markdown
Member

@facekapow left couple of places where there are a few statements on the single line.

@facekapow
Copy link
Copy Markdown
Contributor Author

@iefserge Well, that's the last time I use a beautifier 😄

@iefserge
Copy link
Copy Markdown
Member

LGTM
great work 👍

@kesla
Copy link
Copy Markdown
Member

kesla commented Jul 29, 2016

wow. Amazing.

@facekapow
Copy link
Copy Markdown
Contributor Author

Great, then I'll just do a quick once-over on the code and merge.

@facekapow
Copy link
Copy Markdown
Contributor Author

Everything looks ok, I tried to run the JS tests, but those are definitely broken (more specifically, tape kept complaining about Stream), but I did some manual feature testing and everything looks fine. Merge?

@vitkarpov
Copy link
Copy Markdown

vitkarpov commented Jul 29, 2016

bye-bye history of changes :)

@iefserge
Copy link
Copy Markdown
Member

@facekapow yeah, I've been looking into tests issue, should be fine, going to fix it afterwards.
@vitkarpov yeah, but consistent and nice code style is good :)

@facekapow facekapow merged commit 7299d5e into master Jul 29, 2016
@facekapow facekapow deleted the code-style branch July 29, 2016 18:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants